CFE-885:Feature route external certificate validation by chiragkyal · Pull Request #407 · openshift/openshift-apiserver

chiragkyal · 2023-11-30T12:09:15Z

Description

Updates route validations based on openshift/enhancements#1307 which introduces a new field in the route API externalCertificate behind the TP feature gate.

Changes

ValidateRoute() has been updated to pass additional args (secrets lister and subjectaccessreviewer) to validate externalCertificate and the rbac required.
ValidateHostUpdate() has also been updated since updating route host/subdomain is also affected by updating
certificates on the route.
ValidateHostExternalCertificate() has been introduced as part of the validations done during route updates, this function specifically checks if the user has the correct permissions on the custom-host sub-resource. Additional details can be found on the EP.

Follow up from: openshift/library-go#1625
Featuregate wiring : #382

openshift-ci-robot · 2023-11-30T12:09:19Z

@chiragkyal: This pull request references CFE-885 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.15.0" version, but no target version was set.

Details

In response to this:

Description

Updates route validations based on openshift/enhancements#1307 which introduces a new field in the route API externalCertificate behind the TP feature gate.

Changes

ValidateRoute() has been updated to pass additional args (secrets lister and subjectaccessreviewer) to validate externalCertificate and the rbac required.

ValidateHostUpdate() has also been updated since updating route host/subdomain is also affected by updating
certificates on the route.

ValidateHostExternalCertificate() has been introduced as part of the validations done during route updates, this function specifically checks if the user has the correct permissions on the custom-host sub-resource. Additional details can be found on the EP.

Follow up from: openshift/library-go#1625

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

chiragkyal · 2023-11-30T12:10:54Z

Continue : #385

chiragkyal · 2023-11-30T13:31:18Z

/cc @alebedev87

chiragkyal · 2023-11-30T14:05:29Z

/assign @alebedev87

chiragkyal · 2024-01-25T12:46:27Z

/assign @vrutkovs

openshift-ci-robot · 2024-02-08T17:43:03Z

@chiragkyal: This pull request references CFE-885 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

Details

In response to this:

Description

Updates route validations based on openshift/enhancements#1307 which introduces a new field in the route API externalCertificate behind the TP feature gate.

Changes

ValidateRoute() has been updated to pass additional args (secrets lister and subjectaccessreviewer) to validate externalCertificate and the rbac required.

ValidateHostUpdate() has also been updated since updating route host/subdomain is also affected by updating
certificates on the route.

ValidateHostExternalCertificate() has been introduced as part of the validations done during route updates, this function specifically checks if the user has the correct permissions on the custom-host sub-resource. Additional details can be found on the EP.

Follow up from: openshift/library-go#1625
Featuregate wiring : #382

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

chiragkyal · 2024-04-11T15:06:32Z

Looks like the CIs are failing due to Kube 1.29 rebase issue.
There is a open PR for kube bump: #421

chiragkyal · 2024-04-15T13:47:03Z

Created a PR #425 for Kube 1.29 bump, which should unblock this PR.

Signed-off-by: chiragkyal <ckyal@redhat.com>

Miciah · 2024-04-20T04:19:02Z

I ran make update with this branch checked out on my system, and it did update pkg/openapi/zz_generated.openapi.go. Miciah@084ea84 has the result.

chiragkyal · 2024-04-22T12:38:27Z

I had my forked repo cloned into ~/go/src/github.com/chiragkyal/openshift-apiserver and openapi-gen was not working as expected. After moving that fork into ~/go/src/github.com/openshift/openshift-apiserver, make update did change the same file what @Miciah suggested.

Miciah · 2024-04-22T21:10:01Z

/lgtm

chiragkyal · 2024-04-23T06:33:51Z

/test e2e-aws-ovn-serial

p0lyn0mial

Are you planing to add an e2e test for this functionality ?

p0lyn0mial · 2024-04-30T09:10:42Z

pkg/cmd/openshift-apiserver/cmd.go

 // RunAPIServer takes the options, starts the API server and waits until stopCh is closed or initial listening fails.
 func (o *OpenShiftAPIServer) RunAPIServer(stopCh <-chan struct{}) error {
-	if err := features.InitializeFeatureGates(feature.DefaultMutableFeatureGate, configv1.FeatureGateRouteExternalCertificate); err != nil {
+	if err := features.InitializeFeatureGates(feature.DefaultMutableFeatureGate, configv1.SelfManaged, configv1.FeatureGateRouteExternalCertificate); err != nil {


Assuming Hypershift runs this instance of the API server, how does it specify its own profile?

@deads2k suggested always using self-managed here.
xref: https://redhat-internal.slack.com/archives/C06EK9ZH3Q8/p1713363742667139?thread_ts=1713363148.309629&cid=C06EK9ZH3Q8

please add a comment explaining why to this PR.

please add a comment explaining why to this PR.

Arbitrary choice since this binary doesn't care and teaching the operand about hypershift is not likely to end well.

pkg/route/apis/route/validation/validation.go

p0lyn0mial · 2024-04-30T09:25:58Z

pkg/cmd/openshift-apiserver/openshiftapiserver/config.go

 			AdditionalTrustedCA:                caData,
 			RouteAllocator:                     routeAllocator,
-			AllowRouteExternalCertificates:     feature.DefaultFeatureGate.Enabled(featuregate.Feature(configv1.FeatureGateRouteExternalCertificate)),
+			AllowRouteExternalCertificates:     feature.DefaultMutableFeatureGate.Enabled(featuregate.Feature(configv1.FeatureGateRouteExternalCertificate)),


why did you change it ?

please bring back the previous version.

Looks like both of them carry same thing internally

https://github.com/kubernetes/kubernetes/blob/c91913924543e1d29f3f3d51354701df9df75def/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go#L32

Is it suggested to use DefaultFeatureGate over DefaultMutableFeatureGate ? because we've used DefaultMutableFeatureGate in kube-apiserver

DefaultMutableFeatureGate exposes a different interface. DefaultFeatureGate is read-only, it is not a big deal but I prefer DefaultFeatureGate.

Okay, changed to DefaultFeatureGate

p0lyn0mial · 2024-04-30T09:36:18Z

pkg/route/apis/route/validation/validation.go

-
 // ValidateRoute tests if required fields in the route are set.
-func ValidateRoute(route *routeapi.Route, opts RouteValidationOptions) field.ErrorList {
+func ValidateRoute(ctx context.Context, route *routeapi.Route, sarc routecommon.SubjectAccessReviewCreator, secretsGetter corev1client.SecretsGetter, opts routecommon.RouteValidationOptions) field.ErrorList {


The units we have in strategy_test are going to validate ValidateRoute and ValidateRouteUpdate, right ?

Yes, strategy_test are going to validate ValidateRoute and ValidateRouteUpdate

https://github.com/openshift/openshift-apiserver/blob/master/pkg/route/apiserver/registry/route/strategy_test.go#L475-L478

p0lyn0mial · 2024-04-30T10:12:28Z

pkg/route/apiserver/registry/route/strategy.go

 	route := obj.(*routeapi.Route)
-	errs := routehostassignment.AllocateHost(ctx, route, s.sarClient, s.hostnameGenerator)
-	errs = append(errs, validation.ValidateRoute(route, s.routeValidationOptions())...)
+	errs := routehostassignment.AllocateHost(ctx, route, s.sarClient, s.hostnameGenerator, s.routeValidationOptions())


@benluddy why hasn't this been added as a normal admission plugin?

okay, it looks like this code was added in openshift/kubernetes#1904 and it seems it could be refactored.

p0lyn0mial · 2024-04-30T10:25:53Z

pkg/api/validation/register.go

 func routeOptionsAdapter(route *routeapi.Route) field.ErrorList {
-	return routevalidation.ValidateRoute(route, routevalidation.RouteValidationOptions{})
+	a := &authorizationclient.AuthorizationV1Client{}
+	return routevalidation.ValidateRoute(context.Background(), route, a.SubjectAccessReviews(), &v1.CoreV1Client{}, routecommon.RouteValidationOptions{})
 }

 func routeUpdateOptionsAdapter(route *routeapi.Route, oldRoute *routeapi.Route) field.ErrorList {
-	return routevalidation.ValidateRouteUpdate(route, oldRoute, routevalidation.RouteValidationOptions{})
+	a := &authorizationclient.AuthorizationV1Client{}
+	return routevalidation.ValidateRouteUpdate(context.Background(), route, oldRoute, a.SubjectAccessReviews(), &v1.CoreV1Client{}, routecommon.RouteValidationOptions{})
 }


Used by units in

openshift-apiserver/pkg/api/validation/validation_test.go

Line 1 in 12686d8

package validation

(for validating object's metadata), we should be fine.

p0lyn0mial · 2024-04-30T10:45:06Z

/approve
/hold

until you resolve some of the comments, also it would be nice if you could add an e2e test for this functionality.

p0lyn0mial · 2024-04-30T12:01:29Z

please squash the new commits into [CFE-885: Update route validation and admission to honour externalCertificate

…ificate Signed-off-by: chiragkyal <ckyal@redhat.com>

chiragkyal · 2024-04-30T12:56:19Z

Are you planing to add an e2e test for this functionality ?

We are planning to add e2e tests while graduating this feature to GA.

Ref: https://github.com/openshift/enhancements/blob/master/enhancements/ingress/route-secret-injection-for-external-certificate-management.md#graduation-criteria

chiragkyal · 2024-04-30T13:01:35Z

please squash the new commits into [CFE-885: Update route validation and admission to honour externalCertificate

Done

Are we good to unhold and merge?

p0lyn0mial · 2024-04-30T14:05:56Z

pkg/route/apis/route/validation/validation.go

-
 // ValidateRoute tests if required fields in the route are set.
-func ValidateRoute(route *routeapi.Route, opts RouteValidationOptions) field.ErrorList {
+func ValidateRoute(ctx context.Context, route *routeapi.Route, sarClient routecommon.SubjectAccessReviewCreator, secretsGetter corev1client.SecretsGetter, opts routecommon.RouteValidationOptions) field.ErrorList {


As a final comment, create and update require a lot of SAR requests, but that was agreed upon, described in the proposal and added to the library-go code - https://github.com/thejasn/enhancements/blob/d4c7701193646105e2c60b2b996047ddfed083a4/enhancements/ingress/route-secret-injection-for-external-certificate-management.md

p0lyn0mial · 2024-04-30T14:06:18Z

/hold cancel
/lgtm

openshift-ci · 2024-04-30T14:10:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chiragkyal, Miciah, p0lyn0mial

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [p0lyn0mial]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2024-04-30T15:56:42Z

@chiragkyal: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2024-04-30T22:46:22Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-openshift-apiserver-container-v4.17.0-202404302014.p0.gdd8658e.assembly.stream.el9 for distgit ose-openshift-apiserver.
All builds following this will include this PR.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 30, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2023

openshift-ci bot requested review from bparees and knobunc November 30, 2023 12:10

chiragkyal force-pushed the feature/route-externalcertificate-validation branch from 49d5243 to 532bd3b Compare November 30, 2023 13:28

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2023

openshift-ci bot requested a review from alebedev87 November 30, 2023 13:31

openshift-ci bot assigned alebedev87 Nov 30, 2023

chiragkyal mentioned this pull request Jan 17, 2024

CFE-885:Feature route external certificate validation #385

Closed

openshift-ci bot assigned vrutkovs Jan 25, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2024

chiragkyal force-pushed the feature/route-externalcertificate-validation branch from d4a6cd5 to 230d388 Compare February 8, 2024 17:48

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2024

chiragkyal force-pushed the feature/route-externalcertificate-validation branch from ac0a1da to a391162 Compare February 16, 2024 09:22

chiragkyal force-pushed the feature/route-externalcertificate-validation branch 3 times, most recently from ab26c65 to 5234209 Compare April 7, 2024 16:00

chiragkyal mentioned this pull request Apr 11, 2024

Proof for https://github.com/openshift/kubernetes-apiserver/pull/56 #421

Closed

chiragkyal mentioned this pull request Apr 17, 2024

CFE-1048: Bump Kubernetes to 1.29.2 #425

Merged

chiragkyal added 2 commits April 18, 2024 11:29

Bump openshift/api,client-go,library-go

a23830d

Signed-off-by: chiragkyal <ckyal@redhat.com>

react on featuregate change and error after bump

00e9a02

Signed-off-by: chiragkyal <ckyal@redhat.com>

chiragkyal force-pushed the feature/route-externalcertificate-validation branch from 5234209 to dfe5716 Compare April 18, 2024 06:11

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2024

p0lyn0mial reviewed Apr 30, 2024

View reviewed changes

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels Apr 30, 2024

chiragkyal added 2 commits April 30, 2024 18:20

CFE-885: Update route validation and admission to honour externalCert…

baedcfe

…ificate Signed-off-by: chiragkyal <ckyal@redhat.com>

run make update

74f2fc1

chiragkyal force-pushed the feature/route-externalcertificate-validation branch from 595df6e to 74f2fc1 Compare April 30, 2024 12:52

p0lyn0mial reviewed Apr 30, 2024

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 30, 2024

openshift-ci bot assigned p0lyn0mial Apr 30, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2024

openshift-merge-bot bot merged commit dd8658e into openshift:master Apr 30, 2024

chiragkyal deleted the feature/route-externalcertificate-validation branch April 30, 2024 17:00

gcs278 mentioned this pull request Apr 30, 2024

OCPBUGS-26498: Vendor route API change for UnservableInFutureVersions status #414

Closed

swghosh mentioned this pull request Aug 14, 2024

CM-377: Upstream 1.15 Bump openshift/cert-manager-operator#195

Merged

Miciah mentioned this pull request Sep 23, 2024

OCPBUGS-34373: routes/custom-host permission update for externalCertificate openshift/enhancements#1652

Merged

Conversation

chiragkyal commented Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Uh oh!

openshift-ci-robot commented Nov 30, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Uh oh!

chiragkyal commented Nov 30, 2023

Uh oh!

chiragkyal commented Nov 30, 2023

Uh oh!

chiragkyal commented Nov 30, 2023

Uh oh!

chiragkyal commented Jan 25, 2024

Uh oh!

openshift-ci-robot commented Feb 8, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Uh oh!

chiragkyal commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chiragkyal commented Apr 15, 2024

Uh oh!

Miciah commented Apr 20, 2024

Uh oh!

chiragkyal commented Apr 22, 2024

Uh oh!

Miciah commented Apr 22, 2024

Uh oh!

chiragkyal commented Apr 23, 2024

Uh oh!

p0lyn0mial left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial commented Apr 30, 2024

Uh oh!

p0lyn0mial commented Apr 30, 2024

Uh oh!

chiragkyal commented Apr 30, 2024

Uh oh!

chiragkyal commented Apr 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial commented Apr 30, 2024

chiragkyal commented Nov 30, 2023 •

edited

Loading

openshift-ci-robot commented Nov 30, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Feb 8, 2024 •

edited by openshift-ci bot

Loading

chiragkyal commented Apr 11, 2024 •

edited

Loading